-
-
Notifications
You must be signed in to change notification settings - Fork 901
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: support Encoding class for xml write_to #2798
fix: support Encoding class for xml write_to #2798
Conversation
@ellaklara Thank you for contributing this! I've kicked off CI. I think we should support this in the HTML5 version as well. If there aren't tests we should definitely add that coverage. I'll take a deeper look in the next day or so. |
OK! I've looked into this a bit more, which was good for me because the tests for serialization encoding are somewhat implicit and sort of spread all over the place, so I'd like to add a bunch of test coverage to go along with this. So backing up for a minute to re-describe the problem from #2774 in a bit more detail,
And as you pointed out, we also need to test the HTML5 patch for serialization that lives in So I think the approach to testing I'd like to take is:
Let me add a commit to this PR with some explicit tests for the current behavior, if you don't mind? Then, if you're up for it, I'd love for you to add the Encoding tests for the other methods and finish the feature! |
Aaaand of course I think I'm finding encoding bugs while writing these tests. 😬 |
OK, bug report filed at #2801 But I think that's not material to what we're trying to do in this PR! So ... onward |
9ca7f96
to
7696d6a
Compare
@ellaklara OK! I've added a commit on this PR (before yours) that introduces a new file, Again, only if you're up for it, it would be great to add |
I'm working on the failing JRuby tests, so don't worry about those! Edit: see #2803, I've rebased this PR on top of those commits. |
7696d6a
to
89e49c8
Compare
89e49c8
to
e7b641b
Compare
@ellaklara It's been about a week, so I'm going to go ahead and finish the work you so awesomely started! Will be sure to credit you in the CHANGELOG! |
e7b641b
to
a978f75
Compare
This is what libxml2's `xmlsave.c` functions do, so let's make it explicit in the caller so that JRuby has the same behavior.
Co-authored-by: Mike Dalessio <[email protected]>
ab3c496
to
81faf60
Compare
Thank you, @ellaklara ! |
What problem is this PR intended to solve?
Fixes #2774
Have you included adequate test coverage?
The PR adds two tests that cover both setting encoding with a string and the
Encoding
class.Question
I discovered that the html5 node
write_to
(lib/nokogiri/html5/node.rb
) also doesn't support theEncoding
class, is this something you'd like me to add as well? Where would be a good place to test this? I couldn't find any encoding tests for this class.